Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(kustomize): add enable-helm flag for kustomize4 #947

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

mazzy89
Copy link
Contributor

@mazzy89 mazzy89 commented Feb 3, 2023

This PR introduces the possibility to add the enable-flag to Kustomize.

TODO:

  • check whether the enable-helm is available only for Kustomize4. In that case gate the add function The enable-helm flag is available only for Kustomize4. Need to make sure to have this flag in place only for Kustomize4

@dbyron-sf
Copy link
Contributor

Sorry, I could use some background here. What's the motivation to add --enable-helm when invoking kustomize? What problem does it solve?

@dbyron-sf
Copy link
Contributor

Aaah, I see spinnaker/spinnaker#6731....

@mazzy89
Copy link
Contributor Author

mazzy89 commented Feb 3, 2023

@dbyron-sf many time is requested to extend upstream Helm charts with other resources. more often it is not suitable to modify the upstream helm charts. This flag empowers Kustomize to define upstream helm charts which can be consequently extended using the power of Kustomize and reducing the cost and time to implement any changes into Helm charts.

@mazzy89 mazzy89 marked this pull request as ready for review February 4, 2023 08:32
@mazzy89
Copy link
Contributor Author

mazzy89 commented Feb 4, 2023

@dbyron-sf not sure whether we want to unit test the following change. I haven't seen any other cases. Please advise whether it is enough. I would then make changes into Orca.

@dbyron-sf
Copy link
Contributor

@dbyron-sf not sure whether we want to unit test the following change. I haven't seen any other cases. Please advise whether it is enough. I would then make changes into Orca.

I'd like to see a test that verifies that the command line arg shows up when the flag is set, and the renderer is appropriate. Perhaps HelmTemplateUtilsTest can provide some inspiration for a KustomizeTemplateUtilsTest?

@mazzy89
Copy link
Contributor Author

mazzy89 commented Feb 6, 2023

Sure. Since groovy has been marked as deprecated and no Java tests are present for kustomize, where would you want to see the tests? Should I add it anyway under groovy or create it as Java? @dbyron-sf

@mazzy89 mazzy89 changed the title chore(kustomize): add enable-helm flag chore(kustomize): add enable-helm flag for kustomize4 Feb 6, 2023
@dbyron-sf
Copy link
Contributor

Sure. Since groovy has been marked as deprecated and no Java tests are present for kustomize, where would you want to see the tests? Should I add it anyway under groovy or create it as Java? @dbyron-sf

Same package as the source file (rosco-manifests/src/main/java/com/netflix/spinnaker/rosco/manifests/kustomize/KustomizeTemplateUtils.java), and yes, in java please, so src/test/java/...

mazzy89 and others added 3 commits March 31, 2023 20:09
Signed-off-by: salvatoremazz <salvatoremazz@double.cloud>
Signed-off-by: salvatoremazz <salvatoremazz@double.cloud>
…fests/kustomize/KustomizeBakeManifestRequest.java

Co-authored-by: David Byron <82477955+dbyron-sf@users.noreply.github.com>
@mazzy89 mazzy89 force-pushed the kustomize-enable-helm branch from 141b4cd to 82ef301 Compare March 31, 2023 18:09
@rbadillo
Copy link

@mazzy89 what changes are needed in the Orca side?

Do you have a PR we can take a look?

@mazzy89
Copy link
Contributor Author

mazzy89 commented Aug 31, 2023

Not yet. I do not have an Orca PR ready.

@Benly-walter
Copy link

Any updates on this please?
This feature will improve our k8s deployment setup a whole lot.

@mazzy89
Copy link
Contributor Author

mazzy89 commented Jun 27, 2024

@Benly-walter no updates on this. We have introduced helmfile last year and helmfile can also manage kustomize. I do not see big reasons to continue with that.

@Benly-walter
Copy link

@Benly-walter no updates on this. We have introduced helmfile last year and helmfile can also manage kustomize. I do not see big reasons to continue with that.

Thanks for the update.
In our setup, we use kustomize to manage in-house apps and also for deployment of Helm apps. This means that we do not have any helm releases to maintain and would like to continue with the same approach as it works seamlessly for all our k8s clusters. For these reasons, we do not want to change to using a helmfile.

The bake manifests task works perfect for in-house apps. If the --enable-helm switch is made available for kustomization of a helm chart, it will help greatly simplify our setup. We currently have to rely on custom scripts to check if the deployments have rolled out successfully.

On a different note, do you know if it is also possible to override the below kubectl defaults which are set to false?

--server-side
--force-conflicts

@jasonmcintosh
Copy link
Member

IF someone wants to recreate this PR & add tests ... I can review! :) Doesn't look like it'd be too hard to do...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants